Skip to content

Fix BinaryCodec temporary resource cleanup on errors - #365

Draft
linjiaye929 wants to merge 1 commit into
InterDigitalInc:masterfrom
linjiaye929:fix-binary-codec-error-cleanup
Draft

linjiaye929 wants to merge 1 commit into
InterDigitalInc:masterfrom
linjiaye929:fix-binary-codec-error-cleanup

Conversation

@linjiaye929

Copy link
Copy Markdown

BinaryCodec currently opens two mkstemp descriptors and removes its temporary files only after successful encoding, decoding and image loading. When an external encoder fails, run_command raises SystemExit and bypasses that cleanup.

This is reproducible on the unmodified upstream head through JPEG2000.run(): FFmpeg/libopenjpeg rejects a valid 1×1 PNG because its default resolution count exceeds the image dimensions. Each failed call leaves two open descriptors and two files. Two calls leave four of each. This is independent of the AV1 input-path fix in #364; no AV1 code is changed here.

Use a TemporaryDirectory for BinaryCodec's encoded output and reconstructed PNG, closing their mkstemp descriptors immediately and cleaning up on both successful and exceptional exits. The files are still pre-created with the same suffixes and permissions. Command construction and exception propagation are unchanged.

Validation with the real FFmpeg/libopenjpeg codec:

  • The new failure regression fails on the original head with two leaked descriptors; the success regression passes.
  • After the fix, both regressions and the existing benchmark CLI test pass: 3 passed, no skips.
  • Tests check temporary files and, on Linux, matching open descriptors. The success case verifies reconstruction and metrics.
  • Ruff check and format checks pass for both changed files.

BinaryCodec is shared by JPEG2000, BPG and TFCI. Only JPEG2000 received real codec integration tests; BPG/TFCI tools and a decoder-specific failure were not tested. The existing JPEG binary-fixture test, full suite and upstream CI are not verified. VTM/HM/AV1 have separate implementations and are unchanged.

Prepared with AI assistance. Keeping this as a draft pending broader validation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant